-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api: return 404 for alloc FS list/stat endpoints #11482
Conversation
If the alloc filesystem doesn't have a file requested by the List Files or Stat File API, we currently return a HTTP 500 error with the expected "file not found" error message. Return a HTTP 404 error instead.
79fea58
to
7e8c154
Compare
This missed 1.2.0-rc1, so leaving this in draft state until after the 1.2.0 GA goes out. |
With the help of the UI team, I tracked down some UI related code and acceptance tests that I think will have to change or be deleted along with or shortly after this PR. cc @ChaiWithJai @lgfa29 |
I can follow-up with @ChaiWithJai and try my hand at a little bit of UI work 😀 |
I'm on it! |
Ember Asset Size actionAs of 5431400 Files that got Smaller 🎉:
Files that stayed the same size 🤷:
|
Ember Test Audit comparison
|
Ember Test Audit flaky testsEmber Test Audit detected these flaky tests on main:
Ember Test Audit detected these flaky tests on 70acdb6:
|
Previously the FS handler would interpret a 500 status as a 404 in the adapter layer by checking if the response body contained the text or is the response status was 500 and then throw an error code for 404.
70acdb6
to
5431400
Compare
The UI changes LGTM but it's my PR so I can't be the one to approve it 😀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, late to the party 😅
Minor comment on the tests, but definitely not a blocker. But maybe something we should fix in a follow-up PR.
@@ -361,7 +361,8 @@ export default function browseFilesystem({ | |||
...visitSegments({ allocation: this.allocation, task: this.task }), | |||
path: '/what-is-this', | |||
}); | |||
assert.equal(FS.error.title, 'Not Found', '500 is interpreted as 404'); | |||
assert.notEqual(FS.error.title, 'Not Found', '500 is not interpreted as 404'); | |||
assert.equal(FS.error.title, 'Server Error', '500 is not interpreted as 500'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are a bit weird to me. Since the API now returns a proper 404
, the endpoint stub should've been changed to return a 404
as well instead.
The two assert
s are also kind of redundant. If one passes, the other will not fail since the error title can't be equal to two things at the same time.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fixes #11480
If the alloc filesystem doesn't have a file requested by the List Files (
/v1/client/fs/ls
) or Stat File API (/v1/client/fs/stat
), we currently return a HTTP 500 error with the expected "file not found" error message. Return a HTTP 404 error instead.This change doesn't help the streaming endpoints, but all our callers currently call the
/v1/client/fs/stat
endpoint first.